-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Background commands #6587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! I've reviewed the changes and the implementation looks solid overall. The background terminal execution feature is well-integrated with the existing experimental features system and includes comprehensive localization support. I've left some suggestions inline to improve error handling and test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This seems to achieve a similar effect to disabling terminal integration. However, when terminal integration is disabled, the terminal loses some capabilities and becomes "dumb". Certain commands that rely on updating the terminal state, like some watch commands, no longer work. This approach appears to bypass that limitation.
4b45cc2 to
e01ac26
Compare
|
Should this be in experimental or just added to the terminal settings? |
Added to terminal settings. |
|
@roomote-agent please run merge resolver |
|
Hi @hannesrudolph! I see the merge conflicts on this PR. I'm starting to resolve them now. I'll check out the branch, analyze the conflicts, and push the resolution shortly. |
…sruption experiment with existing experiments
e01ac26 to
013fdc7
Compare
|
✅ Merge conflicts resolved successfully! Resolution SummaryI've successfully resolved the merge conflicts for PR #6587. The conflicts were in the experiment configuration files where both the main branch and this PR were adding new experiments. Files resolved (4 conflicts):
Resolution strategy:The main branch had added Status:
The PR is now ready for review once all CI checks pass. |
| }, | ||
| "PREVENT_TERMINAL_DISRUPTION": { | ||
| "name": "Esecuzione del terminale in background", | ||
| "description": "Previene l'interruzione del focus del terminal quando abilitato. I comandi vengono eseguiti in background senza passare automaticamente ai terminal di output. Puoi continuare a lavorare nel tuo terminal corrente mentre Roo esegue comandi, mantenendo il tuo contesto terminal e controllando manualmente quando visualizzare gli output dei comandi." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typographical suggestion: In 'Previene l'interruzione del focus del terminal quando abilitato. I comandi vengono eseguiti in background senza passare automaticamente ai terminal di output.', consider using 'terminale' instead of 'terminal' for consistency with Italian, and if referring to multiple output terminals, 'ai terminali di output' might be more appropriate.
| "description": "Previene l'interruzione del focus del terminal quando abilitato. I comandi vengono eseguiti in background senza passare automaticamente ai terminal di output. Puoi continuare a lavorare nel tuo terminal corrente mentre Roo esegue comandi, mantenendo il tuo contesto terminal e controllando manualmente quando visualizzare gli output dei comandi." | |
| "description": "Previene l'interruzione del focus del terminale quando abilitato. I comandi vengono eseguiti in background senza passare automaticamente ai terminali di output. Puoi continuare a lavorare nel tuo terminale corrente mentre Roo esegue comandi, mantenendo il tuo contesto terminale e controllando manualmente quando visualizzare gli output dei comandi." |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found issues that need attention.
| // Check if PREVENT_TERMINAL_DISRUPTION is enabled | ||
| // This experimental feature allows commands to run in the background without | ||
| // automatically switching focus to the terminal output, improving workflow continuity | ||
| const provider = task.providerRef.deref() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: you already read provider state in executeCommandTool earlier. You can pass a precomputed flag (e.g., preventTerminalDisruptionEnabled) into executeCommand(...) and avoid this additional getState() call. This eliminates an extra await and state drift between askApproval and execution. Also consider capturing telemetry when focus is suppressed.
| ) | ||
|
|
||
| // Only show terminal if PREVENT_TERMINAL_DISRUPTION is not enabled | ||
| if (!preventTerminalDisruption) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding an escape hatch to the options to explicitly force terminal focus when needed, e.g. forceShowTerminal?: boolean. Then honor it in the show logic. This prevents hidden terminals from breaking interactive flows (auth prompts, TUIs).
| }) | ||
| }) | ||
|
|
||
| describe("PREVENT_TERMINAL_DISRUPTION experiment", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test that simulates an interactive command when preventTerminalDisruption is enabled to ensure the output -> ask() flow still proceeds and does not strand waiting for user input while the terminal is hidden. For example, emulate a prompt line in onLine and verify the tool requests feedback and continues.
Like the background edits, added an experimental setting that does not switch terminals when Roo runs a command.
Important
Introduces
preventTerminalDisruptionexperiment to allow background command execution without terminal focus disruption, with tests and localization updates.preventTerminalDisruptionexperiment to prevent terminal focus disruption during command execution.executeCommandTool.ts, checks ifpreventTerminalDisruptionis enabled to decide whether to show the terminal.executeCommand.spec.tsto verify terminal behavior withpreventTerminalDisruptionenabled and disabled.executeCommandTool.preventTerminalDisruption.integration.spec.tsto ensure correct experiment handling.PREVENT_TERMINAL_DISRUPTIONdescriptions in multiple languages.This description was created by
for 013fdc7. You can customize this summary. It will automatically update as commits are pushed.